chore!: refactor MapeoManager to accept Fastify server instance #440

achou11 · 2024-01-18T23:54:35Z

Towards #437

As I'm working on #437 and dealing with more Fastify instantiation and plugins, I'm noticing that there's an ergonomic awkwardness caused by the MapeoManager being responsible for instantiating the media server instance. Basically was becoming more and more prone to defining MediaServer-specific options for the MapeoManager constructor, which didn't make much sense since the manager mostly just passes it along to the media server.

The ideal approach is only needing to provide an instance of the MediaServer to instantiate the MapeoProject. I initially thought that this wasn't possible because some of the plugins we register for the media server are dependent on methods provided by the manager. The flow of operations was roughly:

Instantiate MapeoManager
Manager instantiates MediaServer internally
MediaServer internally registers plugins dependent on MapeoManager methods

Then I realized that even if that's the case, those methods are only needed when we register the relevant plugins. With this in mind, this PR updates the MediaServer and MapeoManager such that we can now have the following flow of operations instead:

Instantiate MediaServer
Instantiate MapeoManager, providing instance of MediaServer
MapeoManager registers plugins dependent on MapeoManager methods

This makes it much easier to integrate new plugins. In the context of the maps-related functionality, none of the implementation is reliant on the MapeoManager so it can be registered outside of the MapeoManager more freely, meaning we don't need to specify plugin-specific opts for the manager that are just relayed. E.g.

const mediaServer = new MediaServer()
// Will register Blobs and Icons plugins internally
const manager = new MapeoManager( { mediaServer, ... })

// Can register manager independent plugins wherever and whenever!
mediaServer.registerPlugin(MapsPlugin, ...)

Remaining questions:

Do we need MapeoManager.startMediaServer() and MapeoManager.stopMediaServer() anymore with this change? Those are just exposing the relevant media server methods but if the media server is available outside the context of the manager, could we just defer to using mediaServer.start() and mediaServer.close()? EDIT: no longer needed
~~Should we rename the MediaServer at this point? Given this note in Add support for serving custom offline maps #437:~~ DONE: renamed to FastifyController and reduced in scope

Rename the MediaServer to something more generic. Maybe something like MapeoHttpServer, ResourceServer, AppServer??

Notes:

Now that the HTTP server is instantiated separately, consumers of this package will need to instantiate the server themselves and then provide that when instantiating the manager. Should probably update any relevant docs with that

src/mapeo-manager.js

src/media-server.js

gmaclennan

This makes a lot of sense, I like how it simplifies things. I'm tempted to go even further, and just make the parameter for MapeoManager to be a fastify instance. Then the getServerAddress(fastify.server) can just be defined in Mapeo code, and what is called MediaServer could just be FastifyController or FastifyStopStart - could break into a separate module if we want. getMediaAddress could just be a private method on MapeoManager, or we could wrap the logic into MapeoProject methods.

I don't think we need to export start/stopMediaServer on the Manager instance - we start and stop the server from the backend, so it doesn't need to be exposed in the API.

Could make a fastify instance a param of FastifyController too, so code would be:

const fastify = Fastify({ logger })
const fastifyController = new FastifyController({ fastify })
const manager = new MapeoManager({ fastify, ... })
fastifyController.start()

A bit verbose maybe, but clear.

achou11 · 2024-01-19T14:12:35Z

@gmaclennan yep makes a lot of sense! will incorporate that suggestion into this PR (won't create a separate module for now)

achou11 · 2024-01-19T19:30:46Z

@gmaclennan made the following changes based on your feedback:

renamed MediaServer to FastifyController
updated MapeoManager to:
- accept Fastify instance as constructor param instead of MediaServer instance
- implement private getMediaAddress() method (used to live in MediaServer)
update relevant tests (file names and tests)
renamed and updated docs

gmaclennan

This looks great, much neater I think than what we had before. The few things I noticed are actually things from before this PR, it's just that reviewing the code I noticed them. Mainly just nit-picks, but there is one thing that should be addressed - the setting of isFastifyStarted which should happen before fastify.listen() to avoid the race condition when the server is started twice in quick succession. Other suggestions aren't blocking.

gmaclennan · 2024-01-24T20:01:32Z

docs/guides/mapeo-http-server.md

+// Create FastifyController instance for managing the starting and stopping the Fastify server (handles it more gracefully and allows pausing and restarting)
+const fastifyController = new FastifyController({ fastify })


I think maybe add something to this about this being optional - on Desktop we would only need to call fastify.listen(), the use of FastifyController is only necessary for mobile where we need to start and stop the server (because Android doesn't like us running a server while the app is in the background)

addressed via 3fb05dd

gmaclennan · 2024-01-24T20:11:03Z

src/fastify-controller.js

+      await this.#fastify.listen({ host: this.#host, port: this.#port })
+      this.#fastifyStarted = true


I know this was copied across - I realise there is a bug here from my original code. This should avoid a race condition where calling startServer() twice before fastify.listen() resolves would throw an error.

Suggested change

await this.#fastify.listen({ host: this.#host, port: this.#port })

this.#fastifyStarted = true

this.#fastifyStarted = true

await this.#fastify.listen({ host: this.#host, port: this.#port })

addressed via a772a42

gmaclennan · 2024-01-24T20:13:37Z

src/fastify-controller.js

+    this.#host = host
+    this.#port = port


Nit: Again, something carried over, but I don't think these need to be instance properties - they are only used within this method.

addressed via f80ac9a

gmaclennan · 2024-01-24T20:23:05Z

src/fastify-plugins/utils.js

+ * @param {import('node:http').Server} server
+ * @returns {Promise<string>}
+ */
+export async function getFastifyServerAddress(server) {
+  const address = server.address()
+
+  if (!address) {
+    await once(server, 'listening')


Again, something from before, which I'm only noticing because I'm reviewing it. I think how it is fine and won't cause issues, but this change makes it a bit more "correct".

Wrapping this function in a pTimeout leaves the once(server, 'listening') hanging. Better to integrate timeout into this function. I only just learnt about AbortSignal.timeout() but it's helpful here.

Suggested change

* @param {import('node:http').Server} server

* @returns {Promise<string>}

*/

export async function getFastifyServerAddress(server) {

const address = server.address()

if (!address) {

await once(server, 'listening')

* @param {import('node:http').Server} server

* @param {{ timeout?: number }} [options]

* @returns {Promise<string>}

*/

export async function getFastifyServerAddress(server, { timeout } = {}) {

const address = server.address()

if (!address) {

await once(server, 'listening', { signal: timeout ? AbortSignal.timeout(timeout) : undefined })

addressed via 3aafbc0

gmaclennan · 2024-01-24T20:27:28Z

src/mapeo-manager.js

+   * @param {'blobs' | 'icons' | 'maps'} mediaType
+   * @returns {Promise<string>}
+   */
+  async #getMediaAddress(mediaType) {


Nit:

Suggested change

async #getMediaAddress(mediaType) {

async #getMediaBaseUrl(mediaType) {

addressed via 29727df

gmaclennan · 2024-01-24T20:29:07Z

src/mapeo-manager.js

+    const base = await pTimeout(getFastifyServerAddress(this.#fastify.server), {
+      milliseconds: 1000,
+    })


if implementing the suggestion above. Also maybe 5 second timeout is safer (in case on a slow phone it takes a while for the server to start)

Suggested change

const base = await pTimeout(getFastifyServerAddress(this.#fastify.server), {

milliseconds: 1000,

})

const base = await getFastifyServerAddress(this.#fastify.server, {

timeout: 5000,

})

addressed via 3aafbc0

achou11 requested a review from gmaclennan January 18, 2024 23:54

achou11 commented Jan 18, 2024

View reviewed changes

src/mapeo-manager.js Outdated Show resolved Hide resolved

src/media-server.js Outdated Show resolved Hide resolved

achou11 changed the title ~~feat!: make MapeoManager less responsible for MediaServer~~ chore!: make MapeoManager less responsible for MediaServer Jan 19, 2024

gmaclennan reviewed Jan 19, 2024

View reviewed changes

achou11 changed the title ~~chore!: make MapeoManager less responsible for MediaServer~~ chore!: refactor MapeoManager to accept Fastify server instance Jan 19, 2024

achou11 requested a review from gmaclennan January 19, 2024 19:26

achou11 force-pushed the ac/media-server-refactor branch from 4fa59a0 to 28a8d7c Compare January 19, 2024 19:28

This was referenced Jan 23, 2024

feat: add method to MapeoManager for getting stable map style json url #445

Closed

feat: support serving static maps #435

Merged

achou11 force-pushed the ac/media-server-refactor branch from 7653a19 to c5707ff Compare January 23, 2024 19:50

achou11 added 6 commits January 23, 2024 17:01

refactor MediaServer and MapeoManager

60269eb

further refactor based on feedback

0a39dd8

update docs

dee0a86

update wording in docs

0a9fbe5

fix export name

d3af758

update test assertion descriptions

7f5cab7

achou11 force-pushed the ac/media-server-refactor branch from c5707ff to 7f5cab7 Compare January 23, 2024 22:01

gmaclennan approved these changes Jan 24, 2024

View reviewed changes

achou11 added 5 commits January 24, 2024 16:43

update docs

3fb05dd

remove host and port instance properties in FastifyController

f80ac9a

small fix in FastifyController.#startServer()

a772a42

add timeout option to getFastifyServerAddress()

3aafbc0

rename private method in MapeoManager

29727df

achou11 merged commit 805a45a into main Jan 24, 2024
6 of 7 checks passed

achou11 deleted the ac/media-server-refactor branch January 24, 2024 22:00

optic-release-automation bot mentioned this pull request Mar 14, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.5 #519

Merged

This was referenced Apr 11, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.6 #558

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.7 #573

Merged

optic-release-automation bot mentioned this pull request Apr 30, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.8 #592

Merged

optic-release-automation bot mentioned this pull request May 9, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.9 #632

Merged

This was referenced Jun 20, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.10 #709

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.11 #711

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.12 #714

Merged

This was referenced Jul 25, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.13 #726

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.14 #740

Merged

optic-release-automation bot mentioned this pull request Aug 13, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.15 #748

Merged

This was referenced Aug 21, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.16 #758

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.17 #787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore!: refactor MapeoManager to accept Fastify server instance #440

chore!: refactor MapeoManager to accept Fastify server instance #440

achou11 commented Jan 18, 2024 •

edited

Loading

gmaclennan left a comment

achou11 commented Jan 19, 2024

achou11 commented Jan 19, 2024

gmaclennan left a comment

gmaclennan Jan 24, 2024

achou11 Jan 24, 2024

gmaclennan Jan 24, 2024

achou11 Jan 24, 2024

gmaclennan Jan 24, 2024

achou11 Jan 24, 2024

gmaclennan Jan 24, 2024

achou11 Jan 24, 2024

gmaclennan Jan 24, 2024

achou11 Jan 24, 2024

gmaclennan Jan 24, 2024

achou11 Jan 24, 2024

		// Create FastifyController instance for managing the starting and stopping the Fastify server (handles it more gracefully and allows pausing and restarting)
		const fastifyController = new FastifyController({ fastify })

		await this.#fastify.listen({ host: this.#host, port: this.#port })
		this.#fastifyStarted = true

	async #getMediaAddress(mediaType) {
	async #getMediaBaseUrl(mediaType) {

chore!: refactor MapeoManager to accept Fastify server instance #440

chore!: refactor MapeoManager to accept Fastify server instance #440

Conversation

achou11 commented Jan 18, 2024 • edited Loading

gmaclennan left a comment

Choose a reason for hiding this comment

achou11 commented Jan 19, 2024

achou11 commented Jan 19, 2024

gmaclennan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 commented Jan 18, 2024 •

edited

Loading